Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

server: existing statistics will not be deleted when putting store && check when store status changing #3200

Merged
merged 7 commits into from
Nov 24, 2020
Merged

server: existing statistics will not be deleted when putting store && check when store status changing #3200

merged 7 commits into from
Nov 24, 2020

Conversation

HunDunDM
Copy link
Member

@HunDunDM HunDunDM commented Nov 24, 2020 •
edited
Loading

Signed-off-by: Zheng Xiangsheng hundundm@gmail.com

What problem does this PR solve?

  • When changing a Tombstone store to Up or Offline, if the cluster-version has been upgraded at this time, the operation should fail.
  • When TiKV restarts, it will PutStore. This operation currently deletes existing statistics and store-limit.

What is changed and how it works?

  • The store of lower version is no longer allowed to change from Tombstone back to Up.
  • Fix the bug that restarting the store will cause store-limit and statistics to be reset.

Check List

Tests

  • Unit test

Related changes

  • Need to cherry-pick to the release branch

Release note

  • The store of lower version is no longer allowed to change from Tombstone back to Up.
  • Fix the bug that restarting the store will cause store-limit and statistics to be reset.

@HunDunDM
server: check the store version when changing a store whose status is…
483a54c 
… tombstone

Signed-off-by: Zheng Xiangsheng <hundundm@gmail.com>
@HunDunDM
update newTestStores
5ba1f42 
Signed-off-by: Zheng Xiangsheng <hundundm@gmail.com>
@HunDunDM
refine PutStore
2334bde 
Signed-off-by: Zheng Xiangsheng <hundundm@gmail.com>
@HunDunDM
refine store limit and stats
c5b5f1d 
Signed-off-by: Zheng Xiangsheng <hundundm@gmail.com>
@HunDunDM
add test
d0bd69e 
Signed-off-by: Zheng Xiangsheng <hundundm@gmail.com>
@HunDunDM HunDunDM added component/api HTTP API. needs-cherry-pick-release-4.0 The PR needs to cherry pick to release-4.0 branch. labels Nov 24, 2020
@HunDunDM HunDunDM self-assigned this Nov 24, 2020
@HunDunDM
Copy link
Member Author

/run-all-tests

@HunDunDM HunDunDM removed the request for review from JmPotato November 24, 2020 03:51
lhy1024
lhy1024 reviewed Nov 24, 2020
View reviewed changes
server/statistics/store.go Outdated
@@ -45,7 +45,9 @@ func NewStoresStats() *StoresStats {
func (s *StoresStats) CreateRollingStoreStats(storeID uint64) {
s.Lock()
defer s.Unlock()
s.rollingStoresStats[storeID] = newRollingStoreStats()
if _, ok := s.rollingStoresStats[storeID]; !ok {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should remove CreateRollingStoreStats，only to use GetOrCreateRollingStoreStats

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@lhy1024
Copy link
Contributor

lhy1024 commented Nov 24, 2020

It also is a part of #3201

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 24, 2020
@HunDunDM
remove CreateRollingStoreStats
83866df 
Signed-off-by: Zheng Xiangsheng <hundundm@gmail.com>
@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 24, 2020
@nolouch
Copy link
Contributor

nolouch commented Nov 24, 2020

/merge

@ti-chi-bot
Copy link
Member

@nolouch: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the tidb-community-bots/ti-community-prow repository.

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 24, 2020
@ti-chi-bot
Copy link
Member

Can merge label has been added.

Git tree hash: 83866df

@ti-chi-bot
Copy link
Member

@HunDunDM: Your PR has out-of-dated and I have automatically updated it for you.
At the same time I will also trigger all tests for you:

/run-all-tests

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the tidb-community-bots/ti-community-prow repository.

@nolouch
Copy link
Contributor

nolouch commented Nov 24, 2020

/merge

@ti-chi-bot
Copy link
Member

@nolouch: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the tidb-community-bots/ti-community-prow repository.

@nolouch nolouch added status/can-merge Indicates a PR has been approved by a committer. and removed status/can-merge Indicates a PR has been approved by a committer. labels Nov 24, 2020
@nolouch
Copy link
Contributor

nolouch commented Nov 24, 2020

/run-all-tests

@ti-chi-bot ti-chi-bot merged commit 57bfeb7 into tikv:master Nov 24, 2020
ti-srebot pushed a commit to ti-srebot/pd that referenced this pull request Nov 24, 2020
@HunDunDM @ti-srebot
cherry pick tikv#3200 to release-4.0
57570ca 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Nov 24, 2020
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #3206

@HunDunDM HunDunDM deleted the store-state-version-check branch November 25, 2020 03:59
ti-chi-bot added a commit that referenced this pull request Nov 26, 2020
@ti-srebot @HunDunDM @ti-chi-bot
server: check the store version when changing tombstone status (#3200) (
226bd72 
#3206)

* cherry pick #3200 to release-4.0

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

* resolve conflict

Signed-off-by: Zheng Xiangsheng <hundundm@gmail.com>

Co-authored-by: 混沌DM <hundundm@gmail.com>
Co-authored-by: Ti Prow Robot <71242396+ti-community-prow-bot@users.noreply.github.com>
@HunDunDM HunDunDM changed the title server: check the store version when changing tombstone status server: existing statistics will not be cleaned up when putting store && check when store status changing Nov 27, 2020
@HunDunDM HunDunDM changed the title server: existing statistics will not be cleaned up when putting store && check when store status changing server: existing statistics will not be deleted when putting store && check when store status changing Nov 27, 2020
@nolouch nolouch mentioned this pull request Nov 27, 2020
Closed
@HunDunDM HunDunDM mentioned this pull request Dec 23, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lhy1024 lhy1024 lhy1024 approved these changes

@disksing disksing disksing approved these changes

@nolouch nolouch Awaiting requested review from nolouch

@rleungx rleungx Awaiting requested review from rleungx

Assignees

@HunDunDM HunDunDM

Labels
component/api HTTP API. needs-cherry-pick-release-4.0 The PR needs to cherry pick to release-4.0 branch. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@HunDunDM @lhy1024 @nolouch @ti-chi-bot @ti-srebot @disksing